drt: total-order priority queue comparators for stdlib-independent results#10816
drt: total-order priority queue comparators for stdlib-independent results#10816oharboe wants to merge 2 commits into
Conversation
…sults Three priority_queue comparators compare only a subset of their struct's fields, so distinct elements can compare equivalent. std::priority_queue pop order for equivalent elements is implementation-defined and differs between libstdc++ and libc++, making routing results depend on which standard library the binary was built against: - GuidePathFinder::Wavefront ignored prev_idx: on cost ties the chosen predecessor (and the traceback path, and which guides survive) varied by stdlib. On sky130hd/gcd this alone made a gcc/libstdc++ build and a clang/libc++ build produce different guide sets (2469 vs 2470 guides) from bit-identical input; with the tie-break the guide output of the two builds is byte-identical. - FlexDRConnectivityChecker::astar's wf has the same defect (prevIdx not compared). - expandBoxes' Wavefront compared only expansions_done, leaving the DRV-region expansion order implementation-defined. Part of the cross-toolchain QoR divergence in The-OpenROAD-Project#10336; full bit-identical detailed routing additionally needs deterministic region-query ordering (rtree node splits sort with std::sort on keys that tie for track-aligned shapes), tracked in that issue. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request establishes a total order in the comparison operators (operator<) for wavefront and search structures across FlexDR.cpp, FlexDR_conn.cpp, and GuideProcessor.h. By comparing secondary and tertiary fields (such as IDs, indices, and directions) when primary fields (like cost or expansions done) are equal, the priority queue pop order becomes fully deterministic and independent of the standard library's heap implementation. There are no review comments, so I have no feedback to provide.
The deterministic tie-breaks change which equal-cost routing solutions are chosen, shifting flow-test metrics on two designs past their limits. Regenerated test/<design>.metrics and .metrics_limits with save_flow_metrics/save_flow_metrics_limits from this PR's CI run artifacts (PR-10816-head/1, ubuntu22.04), so metrics and limits are consistent with what the gating CI environment actually produces. Both designs improve on most limited metrics in that environment (aes: 10 better / 4 worse; jpeg: 10 better / 5 worse vs the previous recorded metrics), including detailed-route tns, worst slacks, clock skew and hold/repair buffer counts. The two limit violations were DRT::max_slew_slack (aes -21.8% vs a -15.6% limit whose own recorded baseline metric was already -20.7%) and DRT::ANT::errors 0->2 on jpeg (zero-margin metric; the same tie-break reshuffle removes an antenna violation on jpeg_sky130hs in other build environments, so this is routing-reshuffle jitter rather than a systematic regression). Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
CI failure analysis and rebaselineThe two
Pushed a rebaseline commit for these two designs, regenerated with Is the PR net better or worse? Net neutral — it's a reshuffleSame-environment A/B (identical toolchain/machine, master vs this PR, all 13 flow designs, limited metrics per
Only DRT-stage metrics move (RSZ/GRT outputs are bit-identical, as expected for a drt-only change), and the deltas are in the 2nd–3rd significant digit (e.g. tns −6.08 → −7.10 worst case, slew slack −3.2% → −1.7% best case). In the CI environment (this run's artifacts vs the recorded baselines): aes_sky130hd improves on 10 limited metrics and degrades on 4; jpeg_sky130hd improves on 10 and degrades on 5 — including large improvements in clock skew (−28% / −38%), hold buffer count (397→348 / 24→1) and DRT tns. The antenna errors are bidirectional jitter, not systematic: jpeg_sky130hd gains 2 violations in the CI environment, while the same change removes an antenna violation on jpeg_sky130hs (2→1) in another build environment. Both survive Two observations for #10336 while doing this
|
|
@hzeller Any thoughts on whether Claude is delusional or not here? |
|
@hzeller This came up somewhere other when trying to switch to LLVM hermetic. |
| bool operator<(const Wavefront& rhs) const | ||
| { | ||
| return expansions_done > rhs.expansions_done; | ||
| // Total order so that priority_queue pop order does not depend on the |
There was a problem hiding this comment.
if we order the field in the struct the way we are interested in them (exapansion_done, id, ..east, ...) we could also just have the compiler take care of implementing the whole spaceship operator to deal with all situations, since we're on c++20:
auto operator <=>(const Wavefront& rhs) const = default;
There was a problem hiding this comment.
Sounds like followup to this PR? I'm not a domain expert here, I'm just running Claude and this surfaced as an instability in testing.
|
Yes, implementing the operator fully is prudent to make it not prone to implementation details. I'd probably just have the compiler auto-generate the spaceship operator to save code, improve readability and possibly get the most performance out of it. |
|
@maliberty Ask: Could you take this off my hands? |
|
@osamahammad21 this needs a secure CI to check for any changes. Please take over. |
3 participants
Part of #10336 — see the analysis there for the full investigation.
Problem
Three
std::priority_queuecomparators in drt compare only a subset of their element's fields, so distinct elements can compare equivalent. Pop order among equivalent elements is implementation-defined, and libstdc++ and libc++ heaps disagree — so routing results depend on which standard library the binary was built against:GuidePathFinder::Wavefrontignoresprev_idx: on cost ties, the chosen predecessor — and with it the traceback path and which guides survive — varies by stdlib.FlexDRConnectivityChecker::astar'swf: same defect (prevIdxnot compared).expandBoxes'Wavefrontcompares onlyexpansions_done, leaving the DRV-region expansion order implementation-defined.Change
Extend the three comparators to total orders over all fields. Behavior within any single build stays deterministic as before; what changes is that the tie-break is now specified instead of inherited from the stdlib's heap.
Evidence
On sky130hd/gcd (ORFS), a gcc 11/libstdc++ CMake build and a clang 22/libc++ Bazel build of the same source produced different guide sets (2469 vs 2470 guides) from bit-identical input
.odb; with theWavefronttie-break the guide processing output of the two builds is byte-identical (content and order). Track assignment and detailed route still differ across stdlibs due to region-query result ordering (rtree node splits sort withstd::sorton keys that tie for track-aligned shapes) — that is a separate, larger discussion tracked in #10336.Verification
bazelisk test //src/drt/test/...— 26/26 pass.make finishruns deterministically (3/3 identical) with both builds.Type of Change
Bug fix (cross-toolchain result divergence)
I have verified that the local build succeeds
My code follows the repository's formatting guidelines
I have signed my commits (DCO).